chore: allow --api-path as option to generate cmd and generate per api/majorversion #3712
Conversation
diegomarquezp
left a comment
diegomarquezp
left a comment
There was a problem hiding this comment.
Thanks for this PR. It will make local development much easier (no need to modify the gen config yaml anymore).
I left a couple of nits and a minor question.
| seen_library_names[library_name] = library.name_pretty | ||
|
|
||
| @staticmethod | ||
| def from_yaml(path_to_yaml: str) -> "GenerationConfig": |
There was a problem hiding this comment.
Are there any logical change in this method?
There was a problem hiding this comment.
No, only moved so it can be called as GenerationConfig.from_yaml(). Provides better readability imo.
| show_default=True, | ||
| help=""" | ||
| Path within the API root (e.g. googleapis) to the API to | ||
| generate/build/configure etc. This is expected to be a major-versioned |
There was a problem hiding this comment.
What's the difference of major-versioned and versioned?
There was a problem hiding this comment.
the term versions are used in a couple of confusing ways. major-versioned refers to the cases where there is a proto-path for it, and we produce a module in Java. (there is also an "api-version" that is specified within proto, for details go/clientlibs-api-versioning-proposal)
| """, | ||
| ) | ||
| @click.option( | ||
| "--api-path", |
There was a problem hiding this comment.
I think this parameter is overlap with library-names and it's fine-gained than it.
Do you plan to remove library-names later?
There was a problem hiding this comment.
Yes, there are overlap. In short term, will support both so current workflow can work unchanged.
After we onboard fully to 1PP, will evaluate and remove redundant options if needed.
| return proto_path | ||
|
|
||
|
|
||
| def ends_with_version(proto_path: str) -> bool: |
There was a problem hiding this comment.
Why do we need a setter method?
There was a problem hiding this comment.
I am not sure I get what you are referring by "setter method"? If it is a typo and you are asking about this method, it is used here to account for cases where dependencies like /type are included as proto-path (e.g., accesscontextmanager)
That said, the exact logic may change in followup PRs as we have a slight change in design, and I have an related incoming PR that may change GenerationConfig.libraries from a list to dict.
|
Thanks for the patience!#3726 has been merged and merging |
|
|
…i/majorversion (#3712) This change allow generator to generate for a specified api-path. When "--api-path" is not specified, fallback to "--library-names", and __behavior for existing command usages should not change__. For now, api-path should only take in path to api/version, e.g. google/cloud/functions/v2
…i/majorversion (#3712) This change allow generator to generate for a specified api-path. When "--api-path" is not specified, fallback to "--library-names", and __behavior for existing command usages should not change__. For now, api-path should only take in path to api/version, e.g. google/cloud/functions/v2
This change allow generator to generate for a specified api-path. When "--api-path" is not specified, fallback to "--library-names", and behavior for existing command usages should not change.
For now, api-path should only take in path to api/version, e.g. google/cloud/functions/v2
--
Manual tests
generate output